Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

alarm: Long press to stop alarm #2222

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vkareh
Copy link
Contributor

@vkareh vkareh commented Jan 9, 2025

This change prevents accidentally turning off alarm by ensuring that there is a deliberate long press, similar to resetting the Timer app.

InfiniSim_2025-01-10_121345

Copy link

github-actions bot commented Jan 9, 2025

Build size and comparison to main:

Section Size Difference
text 373264B 256B
data 948B 0B
bss 22536B 0B

Run in InfiniEmu

@mark9064 mark9064 added the enhancement Enhancement to an existing app/feature label Jan 10, 2025
@vkareh vkareh force-pushed the alarm-long-press branch 3 times, most recently from 5290eae to 1959b3a Compare January 10, 2025 20:19
This change prevents accidentally turning off alarm by ensuring that
there is a deliberate long press, similar to resetting the Timer app.
@mark9064
Copy link
Member

I haven't had a chance to test this with hardware since the ringing screen has been redesigned. Does the progress bar need to move at all? Maybe it could go between the time and the button in the blank space. Also if it looks a bit blocky (again I need to test), it will render more nicely if it is thinner since there will be fewer pixels to push.

Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially with the bigger button, I think this is a good addition. Thanks for the contribution! The only suggestion I have is calculating the bar progress more consistently and making sure it's super clear to the user that they have to hold the button

}

void Alarm::Refresh() {
if (stopBtnPressing && xTaskGetTickCount() > stopBtnPressTime + pdMS_TO_TICKS(150)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xTaskGetTickCount() - stopBtnPressTime > pdMS_TO_TICKS(150) is tolerant to tick count wraparound
Should we start showing the bar earlier than 150ms, as you need to hold the button to dismiss it. In fact, would it be better with no delay at all? A long press isn't super intuitive, so giving immediate feedback will make it clearer that holding it is needed IMO

Copy link

@Saarsk Saarsk Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xTaskGetTickCount() - stopBtnPressTime > pdMS_TO_TICKS(150) is tolerant to tick count wraparound Should we start showing the bar earlier than 150ms, as you need to hold the button to dismiss it. In fact, would it be better with no delay at all? A long press isn't super intuitive, so giving immediate feedback will make it clearer that holding it is needed IMO

I think immediately showing the bar makes perfect sense. It makes it responsive and informative about what's going on.

I also think it would make sense that the button itself becomes the indicator. It's currently pretty small, but I think a dedicated "alarm triggered" screen would make sense anyway and would make room for a bigger dismiss button.

Instead of showing the alarm settings screen with a small red button (I don't think it's necessarily that intuitive to show the alarm settings screen when the alarm goes off). My suggestion would be to show the time of the alarm only, as well as a larger dismiss button which will also acts as a long press indicator.

The red background would gradually transition into a new background color. If you let go before it fully transition, it should reset back to full red again.

A larger button will make the progress bar clearly visible even with a thumb on the button. And I think that using the button background as indicator is probably the most intuitive. 🙂 With a bigger dismiss button it could even say "hold to dismiss alarm".

Copy link
Member

@mark9064 mark9064 Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting the UI has already been changed since #2211 - so the larger button suggestion is already in :)

I like the idea of using the button as the bar, but I've got two concerns: 1) Performance will be poor: as the button is large, rendering will be choppy. 2) The button may be shrunk in the future to accommodate further functionality such a snooze button.

So maybe a thin indicator bar above the button could work? But I'm not sure I love that idea either. Perhaps a progress indicator isn't needed at all, just some feedback that further holding is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first iteration used the button as the bar, but like @mark9064 said: performance was very poor. The second issue was that I could not see the button changing colour (maybe I have fat fingers?) and so there was no feedback whatsoever.

Here's an updated capture:
InfiniSim_2025-03-27_213657


void Alarm::Refresh() {
if (stopBtnPressing && xTaskGetTickCount() > stopBtnPressTime + pdMS_TO_TICKS(150)) {
stopPosition += 15;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we calculate elapsed ticks with xTaskGetTickCount() - stopBtnPressTime, I think it'd be better to calculate the bar position using this. So if we want to hold for say 1 second, we can do something like elapsedTicks * 1000 / configTICK_RATE to get the number of milliseconds the button has been held, and then draw the progress appropriately / stop alerting when more than 1000 milliseconds has passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to an existing app/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants